fix: using ActiveDirectoryServicePrincipalAccessToken does not set password in connection URL#756
Conversation
Linkgoron
changed the title
Linkgoron
changed the title
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes a bug where using ActiveDirectoryServicePrincipalAccessToken authentication did not include the password in the connection URL, causing the driver to reject the connection.
Changes:
- Added
azuread.ActiveDirectoryServicePrincipalAccessTokento the condition that sets the connection URL user/password inConnectionString().
|
@Linkgoron - Thanks for the catch and the clear writeup the diagnosis is spot on. The Before merging, could you add a unit test in |
Assumes microsoft#756 lands before this PR is merged.
Problem:
I know it's technically undocumented, but I saw that there's a PR for adding it to the docs. I tried using it, and faced an issue "Must provide 'password' parameter when using ActiveDirectoryServicePrincipalAccessToken authentication".
When using --authentication-method ActiveDirectoryServicePrincipalAccessToken, the access token passed via -P or SQLCMDPASSWORD is read into connect.Password but never included in the connection URL. ConnectionString() in pkg/sqlcmd/connect.go:128 only sets connectionURL.User for SqlPassword, ActiveDirectoryPassword, ActiveDirectoryServicePrincipal, and ActiveDirectoryApplication.
Since ActiveDirectoryServicePrincipalAccessToken is missing from this condition, the password field in the URL is empty, and go-mssqldb rejects it with the above error.
The downstream GetTokenBasedConnection appears to add fedauth to the query string, and the driver gets params["password"] from the URL userinfo, so I believe that the only gap is that the password never reaches the URL.
Fix:
Add azuread.ActiveDirectoryServicePrincipalAccessToken to the existing condition on line 128 of pkg/sqlcmd/connect.go.